I'm a bit confused by the dataDir / binaryDestination interplay here. To me (and this is how VS Code has been doing it for a while), if binaryDestination is set, it's the single source of truth. We never look at dataDir for the binary. The binary goes to <binaryDestination>/<cliName>. We also fall back to the CODER_BINARY_DESTINATION env var if the setting isn't configured. If neither is set, we default to <globalStoragePath>/<hostname>/bin.

That's the whole thing. No forceDownloadToData, no conditional fallback between two directories. One setting, one location. The implementation is about 8 lines: pathResolver.ts#L34-L41

I think trying to have binaryDestination sometimes look at dataDir as a fallback is where the complexity explodes. The behavior ends up depending on the combination of enableDownloads, enableBinaryDirectoryFallback, and forceDownloadToData, which is really hard to reason about. I'd lean toward: if binaryDestination is set, that's it, dataDir is out of the picture for the binary

I'm a bit confused by the dataDir / binaryDestination interplay here. To me (and this is how VS Code has been doing it for a while), if binaryDestination is set, it's the single source of truth. We never look at dataDir for the binary. The binary goes to <binaryDestination>/<cliName>. We also fall back to the CODER_BINARY_DESTINATION env var if the setting isn't configured. If neither is set, we default to <globalStoragePath>/<hostname>/bin.

That's the whole thing. No forceDownloadToData, no conditional fallback between two directories. One setting, one location. The implementation is about 8 lines: pathResolver.ts#L34-L41

I think trying to have binaryDestination sometimes look at dataDir as a fallback is where the complexity explodes. The behavior ends up depending on the combination of enableDownloads, enableBinaryDirectoryFallback, and forceDownloadToData, which is really hard to reason about. I'd lean toward: if binaryDestination is set, that's it, dataDir is out of the picture for the binary

You are absolutely right. The complexity explodes due to so many controlling flags and options and combinations of them. Now I don't have the full context but it seems like all of these were dragged from the Gateway support. I'll have to dig up the git history and understand the context around why all of them were needed.

Now to your point I think dataDir is the equivalent of globalStoragePath It is a configurable directory with a default location. Of course flags like enable fallback and enable downloads only makes things worse. I would challenge all of this but as @code-asher mentioned, we do need to pay attention to existing customers who depend on these settings.

I think the data dir fallback was not explicitly requested by any customer.

Someone asked for the ability to customize the binary path to a location only the sysadmin could write to, and the setting would also be controlled by the admin, and I think the logic was "well if that binary is bad or missing or out of date we want to be able to still fall back and download a working binary".

But I am not sure anyone relies on that behavior. One problem is, if sysadmins are doing this, usually it is because unauthorized binaries are not allowed to run on the system anyway. So this fallback would not work in any case. Possibly we can safely remove it without affecting anyone.

But yeah we still need dataDir as an equivalent to globalStoragePath, but maybe we can skip the fallback part. idk, we would have to look into it

There is no support in Toolbox but I think we can write our own migration support for these settings. So that should ease out some of the concerns if it ever gets to the point where we decide to simplify. More or less to me it looks like VS Code extension still relies on two paths just that it cascades automatically without the complication of "enable fallback" which BTW is restricted ONLY to access denied errors.

VS Code extension still relies on two paths just that it cascades automatically without the complication of "enable fallback" which BTW is restricted ONLY to access denied errors.

Yes that is my only concern tbh, if binaryDestination is explicitly configured then an accessibility/permission failure should surface to the user rather than silently falling back to dataDir. Otherwise the user has no idea their configured path isn't actually being used. Like if the user explicitly set binaryDestination, respect that intent and fail loudly when it can't be used rather than quietly switching to a different location behind the scenes. (even if there's an explicit setting for it)

Two small things I noticed:

• The settings page still saves via updateBinaryDirectory() which writes to the old BINARY_DIRECTORY key, so BINARY_DESTINATION is never actually populated through the UI even though it's marked as the replacement. Is this intentional as a backwards-compat bridge or just missed during the rename?
• binaryName is fully removed with no fallback (unlike binaryDirectory which has the ?: store[BINARY_DIRECTORY] fallback). Was this setting actually used by anyone?

Everything else looks good to me and I like the much improved version here! Test coverage is solid and the docs are clear!

if binaryDestination is explicitly configured then an accessibility/permission failure should surface to the user rather than silently falling back to dataDir.

This is what happens for any other type of issue. But not in the case access denied exceptions. I'll talk to Atif and see if can get greenlight to simplify this. I tracked down the original change and Asher was right, it was not requested by anyone.
coder/jetbrains-coder#246

The settings page still saves via updateBinaryDirectory() which writes to the old BINARY_DIRECTORY key
It was intentional in original version, but after the rewrite discussed with Asher it became un-intentional. Thx for pointing that out.

binaryName is fully removed with no fallback
It was never configurable from UI (only if someone tampered manually with the settings.json). But at least in Toolbox world, that is not a recommended practice. For example if you screw the json schema by misplacing a "," then Toolbox will truncate the file because it is not lenient in parsing the json.

This is what happens for any other type of issue. But not in the case access denied exceptions. I'll talk to Atif and see if can get greenlight to simplify this. I tracked down the original change and Asher was right, it was not requested by anyone.
coder/jetbrains-coder#246

lol I seem to have predicted the future in that PR:

Hopefully I have not just overcomplicated things though...

Not a blocker but would it make sense that if the binaryDestination is a folder we check coder[.exe] and coder-${os}-${arch}[.exe]? Because I think the binary we download automatically uses the long name but if you manually go to the releases and download the one you want it's just plain coder

Of course people can just point to the binary and that would work but yeah

Not a blocker but would it make sense that if the binaryDestination is a folder we check coder[.exe] and coder-${os}-${arch}[.exe]? Because I think the binary we download automatically uses the long name but if you manually go to the releases and download the one you want it's just plain coder

Of course people can just point to the binary and that would work but yeah

I'm simply afraid of the danger of overwriting a manually downloaded CLI when the version no longer matches. Maybe instead we could check (in the order of priority):

• $binaryDestination/$hostname/coder-$os-$arch[.exe]
• $binaryDestination/$hostname/coder[.exe]

But then it feels like once again we overengineer the CLI resolution 🤔

But then it feels like once again we overengineer the CLI resolution

Def agree with this. I do still kind of wish we could make a brand new setting with placeholders (defaults to $dataDir/$hostname/coder-$os-$arch) then we could support any path without any ambiguity because it would always point to a file.